Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an environment variable and CLI option to enable/disable default caching #11142

Closed
wants to merge 1 commit into from
Closed

Add an environment variable and CLI option to enable/disable default caching #11142

wants to merge 1 commit into from

Conversation

DharmitD
Copy link
Contributor

@DharmitD DharmitD commented Aug 27, 2024 •
edited
Loading

Resolves #11092
Description of your changes:
An environment variable and a CLI option have been added to control whether task-level caching is enabled by default. The CLI flag --execution-caching-enabled-by-default takes precedence over the environment variable KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT. Here's how it works:

  1. CLI Flag Precedence: If the --execution-caching-enabled-by-default flag is provided, it directly controls whether caching is enabled or disabled, setting the environment variable to enabled or disabled accordingly.

  2. Environment Variable Fallback: If the CLI flag is not provided, the environment variable KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT determines the caching behavior. If this variable is set to enabled, caching is enabled; if set to disabled, caching is disabled.

  3. Default Behavior: If neither the CLI flag nor the environment variable is set, caching defaults to enabled.

Example Cases

Case 1: Default Behavior (No CLI Flag, No Environment Variable Set)

  • Command:

    kfp dsl compile --py cache.py --output output-default.yaml

  • Environment Variable: Not set.

  • CLI Flag: Not provided.

  • Outcome: Caching is enabled by default (because the environment variable defaults to enabled).

Case 2: Environment Variable Set to enabled, No CLI Flag

  • Command:

    export KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT=enabled
kfp dsl compile --py cache.py --output output-env-enabled.yaml

  • Environment Variable: Set to enabled.

  • CLI Flag: Not provided.

  • Outcome: Caching is enabled.

Case 3: Environment Variable Set to disabled, No CLI Flag

  • Command:

    export KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT=disabled
kfp dsl compile --py cache.py --output output-env-disabled.yaml

  • Environment Variable: Set to disabled.

  • CLI Flag: Not provided.

  • Outcome: Caching is disabled.

Case 4: CLI Flag Set to enabled, Environment Variable Set to enabled

  • Command:

    export KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT=enabled
kfp dsl compile --py cache.py --output output-flag-env-enabled.yaml --execution-caching-enabled-by-default=enabled

  • Environment Variable: Set to enabled.

  • CLI Flag: Set to enabled.

  • Outcome: Caching is enabled because the CLI flag confirms it, and it takes precedence.

Case 5: CLI Flag Set to disabled, Environment Variable Set to enabled

  • Command:

    export KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT=enabled
kfp dsl compile --py cache.py --output output-flag-disabled-env-enabled.yaml --execution-caching-enabled-by-default=disabled

  • Environment Variable: Set to enabled.

  • CLI Flag: Set to disabled.

  • Outcome: Caching is disabled because the CLI flag takes precedence over the environment variable.

Case 6: CLI Flag Set to enabled, Environment Variable Set to disabled

  • Command:

    export KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT=disabled
kfp dsl compile --py cache.py --output output-flag-enabled-env-disabled.yaml --execution-caching-enabled-by-default=enabled

  • Environment Variable: Set to disabled.

  • CLI Flag: Set to enabled.

  • Outcome: Caching is enabled because the CLI flag takes precedence over the environment variable.

Case 7: CLI Flag Set to disabled, Environment Variable Set to disabled

  • Command:

    export KFP_EXECUTION_CACHING_ENABLED_BY_DEFAULT=disabled
kfp dsl compile --py cache.py --output output-flag-env-disabled.yaml --execution-caching-enabled-by-default=disabled

  • Environment Variable: Set to disabled.

  • CLI Flag: Set to disabled.

  • Outcome: Caching is disabled because both the environment variable and CLI flag indicate that caching should be disabled.

Summary of Precedence and Outcomes

  • Default Behavior: Caching is enabled by default if no environment variable or CLI flag is set.
  • Environment Variable Controls: If the CLI flag is absent, the environment variable dictates whether caching is enabled or disabled.
  • CLI Flag Overrides Environment Variable: The CLI flag takes precedence over the environment variable and sets the final behavior (and the environment variable) accordingly.

This logic provides flexibility while ensuring that the user’s explicit command-line instructions (the CLI flag) take precedence over environment settings.

Checklist:

@DharmitD DharmitD marked this pull request as draft August 27, 2024 19:32
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign connor-mccarthy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@DharmitD DharmitD changed the title WIP: Add an environment variable and CLI option to enable setting disable … WIP: Add an env var and CLI option to disable default caching Aug 27, 2024
gregsheremeta
gregsheremeta reviewed Aug 27, 2024
View reviewed changes
sdk/python/kfp/cli/compile_.py Outdated Show resolved Hide resolved
sdk/python/kfp/cli/compile_.py Outdated Show resolved Hide resolved
sdk/python/kfp/dsl/pipeline_task.py Outdated Show resolved Hide resolved
@DharmitD DharmitD force-pushed the cache-disable-option branch 2 times, most recently from ae2e91e to 950aad2 Compare August 29, 2024 20:18
@DharmitD DharmitD changed the title WIP: Add an env var and CLI option to disable default caching Add an env var and CLI option to disable default caching Aug 29, 2024
@DharmitD DharmitD changed the title Add an env var and CLI option to disable default caching Add an env var and CLI option to enable/disable default caching Aug 29, 2024
@DharmitD DharmitD marked this pull request as ready for review August 29, 2024 20:36
@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Aug 29, 2024
@DharmitD DharmitD changed the title Add an env var and CLI option to enable/disable default caching Add an environment variable and CLI option to enable/disable default caching Aug 29, 2024
@DharmitD
Copy link
Contributor Author

Ready for review.
cc: @gregsheremeta @zijianjoy @chensun

@DharmitD DharmitD force-pushed the cache-disable-option branch 2 times, most recently from 3f7ab0d to 41d6e19 Compare August 29, 2024 21:21
@google-oss-prow google-oss-prow bot added size/S and removed size/M labels Aug 29, 2024
@DharmitD
Copy link
Contributor Author

/retest test-run-all-gcpc-modules

@google-oss-prow Google OSS Prow
Copy link

@DharmitD: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test kubeflow-pipelines-integration-v2
  • /test test-run-all-gcpc-modules

Use /test all to run the following jobs that were automatically triggered:

  • test-run-all-gcpc-modules

In response to this:

/retest test-run-all-gcpc-modules

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@diegolovison
Copy link
Contributor

diegolovison commented Aug 30, 2024 •
edited
Loading

Environment variable values should align with the conventions used by other Python libraries. Typically, 0 is used to represent false, while any other integer is treated as true. Although some approaches allow for more variation in representing true, such as accepting [1, y, yes, true], it's best to maintain consistency by using only 0 for false and any other integer for true.

@hbelmiro hbelmiro mentioned this pull request Aug 30, 2024
Closed
@gregsheremeta
Copy link
Contributor

To @diegolovison 's point, I also noticed this in the compiler today. There's a strong argument to be made for keeping the user interface consistent.

pipelines/sdk/python/kfp/cli/compile_.py

Lines 131 to 135 in 1c7954d

@click.option(
'--disable-type-check',
is_flag=True,
default=False,
help='Whether to disable type checking.')

@DharmitD , how difficult would it be to rework this using a Feature flag?

@DharmitD
Copy link
Contributor Author

Closing as this has been covered in #11222

@DharmitD DharmitD closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

@gregsheremeta gregsheremeta Awaiting requested review from gregsheremeta

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feature] allow setting a default of execution caching disabled via a compiler CLI flag and env var
3 participants
@DharmitD @diegolovison @gregsheremeta